-
Notifications
You must be signed in to change notification settings - Fork 991
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kernel pos index #3228
kernel pos index #3228
Conversation
d36ff75
to
4311559
Compare
7cb74a6
to
ecd7175
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good to me 👍 Few small comments below.
debug!( | ||
"init_kernel_pos_index: {} kernels, took {}s", | ||
kernel_count, | ||
now.elapsed().as_secs() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be interesting to see if there is any difference with the comment above (without looping on an inclusive range).
.kernel_pos_iter()? | ||
.filter(|(_, (_, height))| *height < cutoff_height) | ||
.map(|(key, _)| batch.delete(&key)) | ||
.count(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity I'm wondering if there is a more efficient way to do that. Like some kind of filter_map
or for_each
.
Also we are loosing the error of the batch delete without any logging (I know it's actually the same situation for the output_pos
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean in terms of "efficient"?
We need to iterate over the full set of entries in the db.
Edit: We do not want to maintain this purely in memory. We actually want transactional (lmdb database) semantics to allow efficient "rewind" via the per block kernel "undo list". |
25105bf
to
531506c
Compare
…undo this is conceptually similar to output_pos but we support duplicates rather than spending to remove
store the kernel_pos index when we call apply_kernel
3342a65
to
eec3137
Compare
Closing, replaced with #3273 (which is a more general solution). |
Maintain index of "recent" kernels so we can quickly lookup a kernel MMR pos (and block height) by kernel excess commitment.
This is similar to the existing
output_pos
index.Important difference is outputs can be removed and pruned (after being spent), kernels are never removed. But we only want to maintain an index of "recent" kernels sufficient for the proposed NSKR relative kernel locks.
Compact the kernel pos index when we compact the txhashset. We maintain 2 weeks of kernel history in the index. This covers 7 days of full block history back to the horizon (for rewind purposes) and a further 7 days of kernel history to cover the max NSKR relative kernel lock period.
Populate the index as we go when calling
apply_kernel()
during processing of new full blocks.Add test coverage around chain compaction and interaction with the kernel pos index.
Track an "undo list" for each full block so we can rewind the kernel_pos index as required when rewinding a block.
We do not need to track the kernel_pos index currently as it is unused.
But we need this implemented for efficient handling of NRD kernel lookups.
Once we have the NRD kernel feature variant we can modify the kernel_pos index to only index recent NRD kernels (currently we index all recent kernels).
Related #3273.
If we track
kernel_pos
via the "linked list" impl then we no longer need to maintain the "undo list" per block.We can simply pop entries off the head of the list during rewind, rewinding each kernel in the block, leaving the index in the previous state in the case of duplicates.